Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sui Native bridge #17579

Merged
merged 87 commits into from
May 9, 2024
Merged

Sui Native bridge #17579

merged 87 commits into from
May 9, 2024

Conversation

patrickkuo
Copy link
Contributor

@patrickkuo patrickkuo commented May 8, 2024

Description

Sui Native Bridge v1 for bridging asset from Ethereum to Sui

Test plan

How did you test the new or updated feature?


Release notes

Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.

For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.

  • Protocol: Added Sui native bridge package 0xb to the Sui framework, the bridge object 0x9 will be created in the next epoch after protocol upgrade.
  • Nodes (Validators and Full nodes):
  • Indexer:
  • JSON-RPC:
  • GraphQL:
  • CLI:
  • Rust SDK:

Copy link

vercel bot commented May 8, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
sui-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 9, 2024 7:52pm
3 Ignored Deployments
Name Status Preview Comments Updated (UTC)
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview May 9, 2024 7:52pm
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview May 9, 2024 7:52pm
sui-typescript-docs ⬜️ Ignored (Inspect) Visit Preview May 9, 2024 7:52pm

Copy link
Contributor

@dariorussi dariorussi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks good to me, though I have not checked and cannot check all code.
It is essentially the bridge code as coming from the branch so not too clear what we need to review.
If anybody wanted to take a look please go ahead, we are trying to get it in by today.
A cursory look for the parts you may care about is good too.
Thanks

@@ -13,7 +13,7 @@ use tracing::{info, warn};

/// The minimum and maximum protocol versions supported by this build.
const MIN_PROTOCOL_VERSION: u64 = 1;
const MAX_PROTOCOL_VERSION: u64 = 45;
const MAX_PROTOCOL_VERSION: u64 = 46;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ebmifa do I need to increment the protocol version to 46 for next devnet cut?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are already at 45 in main, why are we skipping to 46?
CC: @amnn

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will revert it back to 45, I didn't know if 45 is already deployed or not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, 45 is already in devnet as of this Monday.

## Description

Describe the changes or additions included in this PR.

## Test Plan

How did you test the new or updated feature?

---
If your changes are not user-facing and do not break anything, you can
skip the following section. Otherwise, please briefly describe what has
changed under the Release Notes section.

### Type of Change (Check all that apply)

- [ ] protocol change
- [ ] user-visible impact
- [ ] breaking change for a client SDKs
- [ ] breaking change for FNs (FN binary must upgrade)
- [ ] breaking change for validators or node operators (must upgrade
binaries)
- [ ] breaking change for on-chain data layout
- [ ] necessitate either a data wipe or data migration

### Release notes
longbowlu and others added 25 commits May 9, 2024 20:40
## Description 

instead of querying the bridge arg on chain every time, we do this in
the beginning of the program and cache it. If the fetch (with retry)
fails, it panics.

## Test plan 

existing tests 

---

## Release notes

Check each box that your changes affect. If none of the boxes relate to
your changes, release notes aren't required.

For each box you select, include information after the relevant heading
that describes the impact of your changes that a user might notice and
any actions they must take to implement updates.

- [ ] Protocol: 
- [ ] Nodes (Validators and Full nodes): 
- [ ] Indexer: 
- [ ] JSON-RPC: 
- [ ] GraphQL: 
- [ ] CLI: 
- [ ] Rust SDK:
## Description 

Removed inaccurate comments. They were correct until we changed the type
of watermark.

## Test plan 

How did you test the new or updated feature?

---

## Release notes

Check each box that your changes affect. If none of the boxes relate to
your changes, release notes aren't required.

For each box you select, include information after the relevant heading
that describes the impact of your changes that a user might notice and
any actions they must take to implement updates.

- [ ] Protocol: 
- [ ] Nodes (Validators and Full nodes): 
- [ ] Indexer: 
- [ ] JSON-RPC: 
- [ ] GraphQL: 
- [ ] CLI: 
- [ ] Rust SDK:
## Description 

(the meat is in `config.rs`)

This PR reworks `BridgeNodeConfig` to make it easier to reason about.
The main changes:
1. add `sui_bridge_chain_id` and `eth_bridge_chain_id`
2. sui related fields are moved to `SuiConfig`
3. eth related fields are moved to `EthConfig`
4. `eth_addresses` is now `eth_bridge_proxy_address`, and when the node
starts, it fetches all contracts's addresses starting from
`EthSuiBridge`

Other than that, i also moved some common test utils around from
`basic.rs` to `test_utils.rs`.

## Test plan 

unit tests

---

## Release notes

Check each box that your changes affect. If none of the boxes relate to
your changes, release notes aren't required.

For each box you select, include information after the relevant heading
that describes the impact of your changes that a user might notice and
any actions they must take to implement updates.

- [ ] Protocol: 
- [ ] Nodes (Validators and Full nodes): 
- [ ] Indexer: 
- [ ] JSON-RPC: 
- [ ] GraphQL: 
- [ ] CLI: 
- [ ] Rust SDK:
## Description

More Move2024 cleanup and formatting.
Moved tests out of `bridge.move` in its own test directory.
We will transition all tests to that model as we work towards 100% test
coverage.

## Test plan

This is mostly tests

---

## Release notes

Check each box that your changes affect. If none of the boxes relate to
your changes, release notes aren't required.

For each box you select, include information after the relevant heading
that describes the impact of your changes that a user might notice and
any actions they must take to implement updates.

- [ ] Protocol:
- [ ] Nodes (Validators and Full nodes):
- [ ] Indexer:
- [ ] JSON-RPC:
- [ ] GraphQL:
- [ ] CLI:
- [ ] Rust SDK:
## Description 

Thanks to @dariorussi for pointing out this very obvious design flaw.
The SuiBridge contract no longer needs reference the WETH contract at
all, and the vault takes care of all wrapping and unwrapping of ETH.

## Test plan 

Updated necessary tests

---------

Co-authored-by: Lu Zhang <[email protected]>
## Description 

When we run multiple bridge tests in the same time, forge may complain
about the conflicts. This PR fixes this problem by using a different
FOUNDRY_OUT folder for each test run.


## Test plan 

CI tests

---

## Release notes

Check each box that your changes affect. If none of the boxes relate to
your changes, release notes aren't required.

For each box you select, include information after the relevant heading
that describes the impact of your changes that a user might notice and
any actions they must take to implement updates.

- [ ] Protocol: 
- [ ] Nodes (Validators and Full nodes): 
- [ ] Indexer: 
- [ ] JSON-RPC: 
- [ ] GraphQL: 
- [ ] CLI: 
- [ ] Rust SDK:
## Description 

This PR adds `eth_contracts_start_block_fallback` in `BridgeNodeConfig`.
The comment describes it enough:
```
    /// The starting block for EthSyncer to monitor eth contracts.
    /// It is required when `run_client` is true. Usually this is
    /// the block number when the bridge contracts are deployed.
    /// When BridgeNode starts, it reads the contract watermark from storage.
    /// If the watermark is not found, it will start from this fallback block number.
    /// If the watermark is found, it will start from the watermark.
    /// this v.s.`eth_contracts_start_block_override`:
    pub eth_contracts_start_block_fallback: Option<u64>,
    /// The starting block for EthSyncer to monitor eth contracts. It overrides
    /// the watermark in storage. This is useful when we want to reprocess the events
    /// from a specific block number.
    /// Note: this field has to be reset after starting the BridgeNode, otherwise it will
    /// reprocess the events from this block number every time it starts.
    #[serde(skip_serializing_if = "Option::is_none")]
    pub eth_contracts_start_block_override: Option<u64>,
```

Overall, this field provides the ability to give a fallback block to
start when no cursor found in DB. While
`eth_contracts_start_block_override` is more intrusive because of its
highest precedence and stickiness (you need to remove it from config
before restarting the node to void it) .

To better test the behavior, i spin out `get_sui_modules_to_watch` and
`get_eth_contracts_to_watch`.

## Test plan 

existing and new unit tests

---

## Release notes

Check each box that your changes affect. If none of the boxes relate to
your changes, release notes aren't required.

For each box you select, include information after the relevant heading
that describes the impact of your changes that a user might notice and
any actions they must take to implement updates.

- [ ] Protocol: 
- [ ] Nodes (Validators and Full nodes): 
- [ ] Indexer: 
- [ ] JSON-RPC: 
- [ ] GraphQL: 
- [ ] CLI: 
- [ ] Rust SDK:
## Description 

As titled 

## Test plan 

How did you test the new or updated feature?

---

## Release notes

Check each box that your changes affect. If none of the boxes relate to
your changes, release notes aren't required.

For each box you select, include information after the relevant heading
that describes the impact of your changes that a user might notice and
any actions they must take to implement updates.

- [ ] Protocol: 
- [ ] Nodes (Validators and Full nodes): 
- [ ] Indexer: 
- [ ] JSON-RPC: 
- [ ] GraphQL: 
- [ ] CLI: 
- [ ] Rust SDK:
## Description

added Result struct to pass error info back to call site.

## Test plan

How did you test the new or updated feature?

---

## Release notes

Check each box that your changes affect. If none of the boxes relate to
your changes, release notes aren't required.

For each box you select, include information after the relevant heading
that describes the impact of your changes that a user might notice and
any actions they must take to implement updates.

- [ ] Protocol:
- [ ] Nodes (Validators and Full nodes):
- [ ] Indexer:
- [ ] JSON-RPC:
- [ ] GraphQL:
- [ ] CLI:
- [ ] Rust SDK:
## Description 

This PR:
1. refactors e2e test utils by introducing `BridgeTestClusterBuilder`
and `BridgeTestCluster`
2. enables the ability to drop spawned BridgeNodes, and restart them
(not in this PR, but this PR makes it easy to do)
3. speeds up the setup by doing things in parallel. This probably cuts
at least 20 seconds for each test.

## Test plan 

existing tests

---

## Release notes

Check each box that your changes affect. If none of the boxes relate to
your changes, release notes aren't required.

For each box you select, include information after the relevant heading
that describes the impact of your changes that a user might notice and
any actions they must take to implement updates.

- [ ] Protocol: 
- [ ] Nodes (Validators and Full nodes): 
- [ ] Indexer: 
- [ ] JSON-RPC: 
- [ ] GraphQL: 
- [ ] CLI: 
- [ ] Rust SDK:
## Description 

This PR adds more events in Rust to match those emitted in bridge.move.
This is one extra step towards the monitoring system on bridge node.
Also adds two two handy functions `new_bridge_transactions` and
`new_bridge_events` to help observe bridge transactions/events.


## Test plan 

unit tests + integration tests

---

## Release notes

Check each box that your changes affect. If none of the boxes relate to
your changes, release notes aren't required.

For each box you select, include information after the relevant heading
that describes the impact of your changes that a user might notice and
any actions they must take to implement updates.

- [ ] Protocol: 
- [ ] Nodes (Validators and Full nodes): 
- [ ] Indexer: 
- [ ] JSON-RPC: 
- [ ] GraphQL: 
- [ ] CLI: 
- [ ] Rust SDK:
…e the message_type field (#17365)

## Description

for better naming and fewer bytes

## Test plan

How did you test the new or updated feature?

---

## Release notes

Check each box that your changes affect. If none of the boxes relate to
your changes, release notes aren't required.

For each box you select, include information after the relevant heading
that describes the impact of your changes that a user might notice and
any actions they must take to implement updates.

- [ ] Protocol:
- [ ] Nodes (Validators and Full nodes):
- [ ] Indexer:
- [ ] JSON-RPC:
- [ ] GraphQL:
- [ ] CLI:
- [ ] Rust SDK:
## Description 

This PR adds events for committee.move in bridge node.

## Test plan 

tests

---

## Release notes

Check each box that your changes affect. If none of the boxes relate to
your changes, release notes aren't required.

For each box you select, include information after the relevant heading
that describes the impact of your changes that a user might notice and
any actions they must take to implement updates.

- [ ] Protocol: 
- [ ] Nodes (Validators and Full nodes): 
- [ ] Indexer: 
- [ ] JSON-RPC: 
- [ ] GraphQL: 
- [ ] CLI: 
- [ ] Rust SDK:
## Description 

added a new command to bridge tool cli to register as a committee member

## Test plan 

manually tested on bridgenet

---

## Release notes

Check each box that your changes affect. If none of the boxes relate to
your changes, release notes aren't required.

For each box you select, include information after the relevant heading
that describes the impact of your changes that a user might notice and
any actions they must take to implement updates.

- [ ] Protocol: 
- [ ] Nodes (Validators and Full nodes): 
- [ ] Indexer: 
- [ ] JSON-RPC: 
- [ ] GraphQL: 
- [ ] CLI: 
- [ ] Rust SDK:
## Description

1. `get_token_transfer_action_signatures` and
`get_token_transfer_action_status` does not need mutable Bridge ref
2. switch the order of Bridge and clock in `claim_token_internal` so we
can use the receiver syntax

## Test plan

existing tests

---

## Release notes

Check each box that your changes affect. If none of the boxes relate to
your changes, release notes aren't required.

For each box you select, include information after the relevant heading
that describes the impact of your changes that a user might notice and
any actions they must take to implement updates.

- [ ] Protocol:
- [ ] Nodes (Validators and Full nodes):
- [ ] Indexer:
- [ ] JSON-RPC:
- [ ] GraphQL:
- [ ] CLI:
- [ ] Rust SDK:
## Description

Mostly moved tests in their own files. Particularly tests that have to
be expanded.
Move 2024 cleanup.
Formatting and convention alignment.

This is almost no change except for few visibility changes we will talk
about

## Test plan

Existing test, no feature added

---

## Release notes

Check each box that your changes affect. If none of the boxes relate to
your changes, release notes aren't required.

For each box you select, include information after the relevant heading
that describes the impact of your changes that a user might notice and
any actions they must take to implement updates.

- [ ] Protocol:
- [ ] Nodes (Validators and Full nodes):
- [ ] Indexer:
- [ ] JSON-RPC:
- [ ] GraphQL:
- [ ] CLI:
- [ ] Rust SDK:
regen snapshots

fix test
## Description

Audit feedback around `notional_value`, minor cleanup

## Test plan

Existing tests

---

## Release notes

Check each box that your changes affect. If none of the boxes relate to
your changes, release notes aren't required.

For each box you select, include information after the relevant heading
that describes the impact of your changes that a user might notice and
any actions they must take to implement updates.

- [ ] Protocol:
- [ ] Nodes (Validators and Full nodes):
- [ ] Indexer:
- [ ] JSON-RPC:
- [ ] GraphQL:
- [ ] CLI:
- [ ] Rust SDK:
fix from the Hacken bug bounty.
move bridge to protocol version 45

update snapshots
@patrickkuo patrickkuo enabled auto-merge (squash) May 9, 2024 20:19
@patrickkuo patrickkuo merged commit d01026e into main May 9, 2024
48 checks passed
@patrickkuo patrickkuo deleted the native-bridge branch May 9, 2024 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants